Inserter: use lighter grammar parse to check allowed status #64902
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ellatrix
added
[Feature] Inserter
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ellatrix
commented
Aug 29, 2024
| @@ -300,10 +300,10 @@ export const hasAllowedPatterns = createRegistrySelector( ( select ) => | |||
| if ( ! inserter ) { | |||
| return false; | |||
| } | |||
| const { blocks } = getParsedPattern( pattern ); | |||
| const grammar = getGrammar( pattern ); | |||
There was a problem hiding this comment.
Grammar parse is lightening fast, but probably still a good idea to cache.
There was a problem hiding this comment.
It's worth measuring the impact if possible, sometimes caching adds more overhead than the actual operation.
ellatrix
commented
Aug 29, 2024
| return ( | ||
| checkAllowListRecursive( blocks, allowedBlockTypes ) && | ||
| blocks.every( ( { name: blockName } ) => | ||
| checkAllowListRecursive( grammar, allowedBlockTypes ) && |
There was a problem hiding this comment.
It's funny that checkAllowListRecursive is already made to work with the grammar objects... makes me wonder if we did that at some point before.
There was a problem hiding this comment.
It looks like it was used when the method was introduced - #30366. We should probably check why it was removed in case there was an issue with grammar parser.
There was a problem hiding this comment.
My guess is that there was a need elsewhere for parsed content, and someone switched it to a full parse. But yes we should try to fish it out of the history.
There was a problem hiding this comment.
It was removed in #32376.
There was a problem hiding this comment.
And it was removed to improve correctness of the block names: fallback names, deprecations that migrate to another block type etc.
There was a problem hiding this comment.
why the underlying parser produces them in the first place
I'm now looking at the code and it's totally intentional!
There is a token of type void-block and it produces block records only on top level. @dmsnell why does the parser do that? It looks like it has something to do with freeform/classic content.
There was a problem hiding this comment.
yes the goal of the parser is to represent the source document, so creating a fallback block for whitespace captures that and lets us write it back.
there was considerable discussion about this a number of years ago. the suggestion in each case is just to ignore the whitespace-only fallback blocks.
There was a problem hiding this comment.
@dmsnell do you remember why the fallback blocks with whitespace are created only on top level, but not in inner blocks?
If the goal of the parser is to represent the source document well, it should also capture whitespace in a nested markup.
Two nice things about using the lightweight parser for parsing patterns:
- patterns are more likely to have correct and modern block markup, because the concept of patterns is new. It's mostly the post content where dealing with legacy old things is important.
- if we ever implement block lazy loading, that means the full two-stage parse must be async (promise-returning) because block definitions are loaded by the parser as it needs them. But the first-state lighter parser can remain completely sync.
There was a problem hiding this comment.
do you remember why the fallback blocks with whitespace are created only on top level, but not in inner blocks?
@jsnajdr are you certain what you are seeing is as you describe it? these aren't "empty blocks" so much as they are HTML content outside of a block, or "HTML soup." the whitespace isn't ignored just because it's inside of another block, but when it's HTML content inside of a block it becomes part of that block's inner HTML and inner content. in fact, this is always the case because the whitespace is just innerHTML
You can explore this at my parser explorer - https://dmsnell.github.io/peg-parser-explorer/ - just make sure to click the Fetch button to load the block grammar.
innerContent is an array containing string chunks of HTML content, and null as a placeholder for inner blocks, allowing the reconstruction of multiple non-nested inner blocks within another block. all whitespace should appear both in innerHTML and in innerContent.
There was a problem hiding this comment.
Thanks for clarification @dmsnell, now I fully understand what's going on. At the top level the whitespace is represented as void blocks, because there is no innerContent field where we could put that information.
For inner blocks, the whitespace is in innerContent, which is an array that looks like ["\n\t", null, "\n\t", null, "\n"]. The whitespace can be fully reconstructed from this. We don't need void blocks here.
There is also innerHTML field which is a string like "\n\t\n\t\n". This field is only useful for "leaf" blocks that have their own markup that's in innerHTML, but for "container" blocks the information is not very useful, as all the whitespace is concatenated together.
ellatrix
commented
Aug 29, 2024
| .map( ( pattern ) => { | ||
| return { | ||
| ...pattern, | ||
| get blocks() { |
There was a problem hiding this comment.
I'm adding this for back compat and also for us. Eventually we should adjust the uses of this selector to not expect blocks and only parse when it's needed.
|
Size Change: +1.52 kB (+0.09%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
ellatrix
added
the
Backport to WP Minor Release
2 tasks
|
Please someone else merge so the props are not forgotten 😅 |
jsnajdr
approved these changes
Aug 30, 2024
bph
pushed a commit
to bph/gutenberg
that referenced
this pull request
Aug 31, 2024
…s#64902) Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
|
@ellatrix I like where this is going; here are some thoughts:
I'm happy to help bring that change into JavaScript, and then it would defer even more processing and memory allocation, but I'm running against a lack of time and over-commitment already, so it would be ideal if I could be a reviewer instead of a lead on that work. The code should be trivial to port. In concept it's replacing the embedded RegExp pattern in the block parser with a function that can be called from the outside. |
vcanales
pushed a commit
that referenced
this pull request
Sep 4, 2024
Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
|
@vcanales and I discussed this PR today for WP 6.6.2.
Conclusion:
Moved this PR back to the Review column in the 6.6.x Editor Task Board, denoting it'll be reassessed for inclusion in the next 6.6.x minor after it's been released in the plugin. Hopefully, by the next 6.6.x minor, more fixes are ready to further help with the performance issue. |
talldan
added a commit
that referenced
this pull request
Sep 4, 2024
* Fix canvas issues by removing VisualEditor’s height (#63724) Unlinked contributors: wp-seopress. Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> * Post Editor: Prevent popover from being hidden by metabox (#63939) * Post Editor: Prevent popover from being hidden by metabox * Use `.interface-interface-skeleton__content` instead of `.interface-interface-skeleton__body` Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: Rishit30G <rishit30g@git.wordpress.org> Co-authored-by: dhananjaykuber <dhananjaykuber@git.wordpress.org> * Global Styles: Fix block custom CSS pseudo element selectors (#63980) Unlinked contributors: harlet. Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: dballari <dballari@git.wordpress.org> Co-authored-by: wongjn <wongjn@git.wordpress.org> * Avoid errors for post types without a 'menu_icon' (#64015) Unlinked contributors: karan4official. Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> * Data Views: Don't render action modal when there are no eligible items (#64250) Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> * Post editor: apply space below content using a pseudo-element instead of padding-bottom (#64639) Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: PC888 <kracked888@git.wordpress.org> * Featured Image Block: Reduce CSS specificity (#64463) Co-authored-by: dsas <dsas@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> * Inserter: use lighter grammar parse to check allowed status (#64902) Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> * Prepare JSON schemas for Draft 7 update (#63582) Co-authored-by: ajlende <ajlende@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org> * Fix missing ref support for textAlign and textColumns in theme.json schema (#63625) * Add missing ref support for textAlign and textColumns * Update the theme.json reference docs Co-authored-by: ajlende <ajlende@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org> * Don't allow duplicating template parts in non-block-based themes (#64379) Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: MadtownLems <madtownlems@git.wordpress.org> * Fix bumped specificity for layout styles in non-iframed editor (#64076) * Fix too specific layout styles in non-iframed editor * Ensure first/last child rules take precedence * Adjust selectors so that `> :first-child`/`> :last-child` still has 0,2,0 specificity to override theme.json spacing * Update tests * Update client side layout selectors to match theme json * Add backport changelog ---- Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> * Update postcss-prefixwrap dependency to 1.51.0 to fix prefixing in `:where` selectors (#64458) * Update postcss-prefixwrap dependency to 1.51.0 to fix prefixing in :where selectors * Add extra test for :where with a pseudo selector ---- Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: andreiglingeanu <andreiglingeanu@git.wordpress.org> * Revert "Update postcss-prefixwrap dependency to 1.51.0 to fix prefixing in `:where` selectors (#64458)" This reverts commit 53a370e. * Update postcss-prefixwrap dependency to 1.51.0 to fix prefixing in `:where` selectors (#64458) * Update postcss-prefixwrap dependency to 1.51.0 to fix prefixing in :where selectors * Add extra test for :where with a pseudo selector ---- Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: andreiglingeanu <andreiglingeanu@git.wordpress.org> * Post Editor: fix click space after post content to append (#64992) * Fix minimally * Revise comment * Stop propagation and don’t prevent default * Insert default block if no blocks are present * re-build package lock to match format * Revert "Inserter: use lighter grammar parse to check allowed status (#64902)" This reverts commit 9b9bbe8. --------- Co-authored-by: Mitchell Austin <mr.fye@oneandthesame.net> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: stokesman <presstoke@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: Rishit30G <rishit30g@git.wordpress.org> Co-authored-by: dhananjaykuber <dhananjaykuber@git.wordpress.org> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: dballari <dballari@git.wordpress.org> Co-authored-by: wongjn <wongjn@git.wordpress.org> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: PC888 <kracked888@git.wordpress.org> Co-authored-by: Dean Sas <dean@deansas.org> Co-authored-by: dsas <dsas@git.wordpress.org> Co-authored-by: Ella <4710635+ellatrix@users.noreply.github.com> Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: Alex Lende <alex+github.com@lende.xyz> Co-authored-by: ajlende <ajlende@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: MadtownLems <madtownlems@git.wordpress.org>
cbravobernal
removed
the
Backport to WP Minor Release
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
The problem: when a site has thousands of patterns, these patterns are all parsed when you open the "Patterns" tab in the inserter. It seems this is needed to check if the pattern is allowed in the selected block (so it needs to know the pattern contents). However, we could use the lighter "grammar" parsing here instead of doing a full parse, since we only need to know the block names that are in the pattern content. This way we can delay full parse until you actually click into a category, and the full parse will only be executed on the visible patterns (not all thousands of them).
Why?
Performance. When there are thousands of patterns, the tab freezes because all have to parsed just to display the categories.
How?
See above.
Testing Instructions
It's hard to test the performance issues locally, but it's easier to understand if you log at the parse() call. You will see that the parsing is executed e.g. 3000 times by simply opening the Patterns tab in the inserter.
Testing Instructions for Keyboard
Screenshots or screencast